[BEEFY] Add runtime support for reporting fork voting #4522

serban300 · 2024-05-20T12:19:28Z

Related to #4523

Extracting part of #1903 (credits to @Lederstrumpf for the high-level strategy), but also introducing significant adjustments both to the approach and to the code. The main adjustment is the fact that the ForkVotingProof accepts only one vote, compared to the original version which accepted a vec![]. With this approach more calls are needed in order to report multiple equivocated votes on the same commit, but it simplifies a lot the checking logic. We can add support for reporting multiple signatures at once in the future.

There are 2 things that are missing in order to consider this issue done, but I would propose to do them in a separate PR since this one is already pretty big:

benchmarks/computing a weight for the new extrinsic (this wasn't present in BEEFY: add support for slashing validators signing forking commitments #1903 either)
exposing an API for generating the ancestry proof. I'm not sure if we should do this in the Mmr pallet or in the Beefy pallet

Co-authored-by: Robert Hambrock roberthambrock@gmail.com

report_equivocation -> report_double_voting

svyatonik

LGTM

substrate/frame/beefy-mmr/src/lib.rs

svyatonik · 2024-05-20T14:01:11Z

substrate/frame/beefy/src/equivocation.rs

 		}
+
+		let evidence = call.to_equivocation_evidence_for().ok_or(InvalidTransaction::Call)?;
+		let tag = (evidence.offender_id().clone(), evidence.set_id(), *evidence.round_number());


Do we need to accept two different types of equivocation reports (double voting and fork voting) at a single slot? If so, we maybe need to add that type to the tag

I don't think we can accept 2 equivocation reports from the same offender in the same round. I think check_evidence will fail:

fn check_evidence( evidence: EquivocationEvidenceFor<T>, ) -> Result<(), TransactionValidityError> { let offender = evidence.checked_offender::<P>().ok_or(InvalidTransaction::BadProof)?; // Check if the offence has already been reported, and if so then we can discard the report. let time_slot = TimeSlot { set_id: evidence.set_id(), round: *evidence.round_number() }; if R::is_known_offence(&[offender], &time_slot) { Err(InvalidTransaction::Stale.into()) } else { Ok(()) } }

Sorry, we could change check_evidence. I don't know what would be best. I would lean towards staying consistent with the current approach where we accept one equivocation per (offender, set_id, round).

I don't see anywhere in this PR the extrinsic used to report these ForkEquivocations..

I am thinking since you only allow single offender per report does that mean that for a slashable fork equivocation we need to report all the offenders across multiple blocks?

I don't see anywhere in this PR the extrinsic used to report these ForkEquivocations..

The extrinsic is here .

I am thinking since you only allow single offender per report does that mean that for a slashable fork equivocation we need to report all the offenders across multiple blocks?

Hmmm good point. Could be. I can try to implement the solution that was discussed here. It's good to have it anyway.

The extrinsic is here .

Right, I got confused. I had noticed we don't have runtime APIs for the "unsigned" version of the call. Should we add it?

Yes, missed that. Will add it.

I am thinking since you only allow single offender per report does that mean that for a slashable fork equivocation we need to report all the offenders across multiple blocks?

Hmmm good point. Could be. I can try to implement the solution that was discussed here. It's good to have it anyway.

Done. PTAL ! Relevant commit: f709cef

Right, I got confused. I had noticed we don't have runtime APIs for the "unsigned" version of the call. Should we add it?

Yes, missed that. Will add it.

On a second thought, I think it's better to do this in a separate PR together with the logic for generating the ancestry proofs since the type of the ancestry proof will depend on that (it could either be AncestryProof or OpaqueValue, depending on how we generate it). WDYT ?

substrate/frame/merkle-mountain-range/src/tests.rs

Lederstrumpf · 2024-05-21T06:53:02Z

The main adjustment is the fact that the ForkVotingProof accepts only one vote, compared to the original version which accepted a vec![]. With this approach more calls are needed in order to report multiple equivocated votes on the same commit, but it simplifies a lot the checking logic. We can add support for reporting multiple signatures at once in the future.

When you state "at once in the future":
Is this an immediate follow-up PR prior to integrating the client side changes from #1903, or possibly delayed beyond then - i.e. adjusting the client side changes in #1903 to work with a single vote prior to moving to a vec (eventually)?

serban300 · 2024-05-23T11:28:15Z

The main adjustment is the fact that the ForkVotingProof accepts only one vote, compared to the original version which accepted a vec![]. With this approach more calls are needed in order to report multiple equivocated votes on the same commit, but it simplifies a lot the checking logic. We can add support for reporting multiple signatures at once in the future.

When you state "at once in the future": Is this an immediate follow-up PR prior to integrating the client side changes from #1903, or possibly delayed beyond then - i.e. adjusting the client side changes in #1903 to work with a single vote prior to moving to a vec (eventually)?

Discussed offline. I am not sure if we should support a batch of votes, since it will complicate things, but if we do, we can do any of the 2 options. They both seem ok to me.

Define `OpaqueValue` and use it instead of `grandpa::OpaqueKeyOwnershipProof` and `beefy:OpaqueKeyOwnershipProof` Related to #4522 (comment) We'll need to introduce a runtime API method that calls the `report_fork_voting_unsigned()` extrinsic. This method will need to receive the ancestry proof as a paramater. I'm still not sure, but there is a chance that we'll send the ancestry proof as an opaque type. So let's introduce this `OpaqueValue`. We can already use it to replace `grandpa::OpaqueKeyOwnershipProof` and `beefy:OpaqueKeyOwnershipProof` and maybe we'll need it for the ancestry proof as well. --------- Co-authored-by: Bastian Köcher <git@kchr.de>

…n-runtime

Define `OpaqueValue` and use it instead of `grandpa::OpaqueKeyOwnershipProof` and `beefy:OpaqueKeyOwnershipProof` Related to paritytech#4522 (comment) We'll need to introduce a runtime API method that calls the `report_fork_voting_unsigned()` extrinsic. This method will need to receive the ancestry proof as a paramater. I'm still not sure, but there is a chance that we'll send the ancestry proof as an opaque type. So let's introduce this `OpaqueValue`. We can already use it to replace `grandpa::OpaqueKeyOwnershipProof` and `beefy:OpaqueKeyOwnershipProof` and maybe we'll need it for the ancestry proof as well. --------- Co-authored-by: Bastian Köcher <git@kchr.de>

…n-runtime

substrate/frame/beefy/src/equivocation.rs

prdoc/pr_4522.prdoc

Co-authored-by: Adrian Catangiu <adrian@parity.io>

prdoc/pr_4522.prdoc

bkontur · 2024-07-03T12:29:22Z

substrate/frame/beefy/src/default_weights.rs

@@ -50,6 +57,11 @@ impl crate::WeightInfo for () {
 			.saturating_add(DbWeight::get().reads(2))
 	}

+	// TODO: Calculate


@serban300 what about this TODO?

I plan to do this in a separate PR, as mentioned in the description. Also keeping track of it in the issue: #4523 to make sure we don't forget.

franciscoaguirre · 2024-07-03T12:31:40Z

prdoc/pr_4522.prdoc

+doc:
+  - audience:
+    - Runtime Dev
+    - Runtime User


Is this description relevant to runtime users?

I'm thinking it might be helpful for them to know of the new extrinsics.

prdoc/pr_4522.prdoc

@Lederstrumpf

Related to paritytech#4523 Extracting part of paritytech#1903 (credits to @Lederstrumpf for the high-level strategy), but also introducing significant adjustments both to the approach and to the code. The main adjustment is the fact that the `ForkVotingProof` accepts only one vote, compared to the original version which accepted a `vec![]`. With this approach more calls are needed in order to report multiple equivocated votes on the same commit, but it simplifies a lot the checking logic. We can add support for reporting multiple signatures at once in the future. There are 2 things that are missing in order to consider this issue done, but I would propose to do them in a separate PR since this one is already pretty big: - benchmarks/computing a weight for the new extrinsic (this wasn't present in paritytech#1903 either) - exposing an API for generating the ancestry proof. I'm not sure if we should do this in the Mmr pallet or in the Beefy pallet Co-authored-by: Robert Hambrock <roberthambrock@gmail.com> --------- Co-authored-by: Adrian Catangiu <adrian@parity.io>

Define `OpaqueValue` and use it instead of `grandpa::OpaqueKeyOwnershipProof` and `beefy:OpaqueKeyOwnershipProof` Related to paritytech#4522 (comment) We'll need to introduce a runtime API method that calls the `report_fork_voting_unsigned()` extrinsic. This method will need to receive the ancestry proof as a paramater. I'm still not sure, but there is a chance that we'll send the ancestry proof as an opaque type. So let's introduce this `OpaqueValue`. We can already use it to replace `grandpa::OpaqueKeyOwnershipProof` and `beefy:OpaqueKeyOwnershipProof` and maybe we'll need it for the ancestry proof as well. --------- Co-authored-by: Bastian Köcher <git@kchr.de>

@Lederstrumpf

Related to paritytech#4523 Extracting part of paritytech#1903 (credits to @Lederstrumpf for the high-level strategy), but also introducing significant adjustments both to the approach and to the code. The main adjustment is the fact that the `ForkVotingProof` accepts only one vote, compared to the original version which accepted a `vec![]`. With this approach more calls are needed in order to report multiple equivocated votes on the same commit, but it simplifies a lot the checking logic. We can add support for reporting multiple signatures at once in the future. There are 2 things that are missing in order to consider this issue done, but I would propose to do them in a separate PR since this one is already pretty big: - benchmarks/computing a weight for the new extrinsic (this wasn't present in paritytech#1903 either) - exposing an API for generating the ancestry proof. I'm not sure if we should do this in the Mmr pallet or in the Beefy pallet Co-authored-by: Robert Hambrock <roberthambrock@gmail.com> --------- Co-authored-by: Adrian Catangiu <adrian@parity.io>

serban300 added 3 commits May 20, 2024 13:25

Add Beefy pallet hook for checking ancestry

e2b72b9

Renamings

8a7ac61

report_equivocation -> report_double_voting

Add support for reporting fork voting

1ff72ef

serban300 added R0-silent Changes should not be mentioned in any release notes T15-bridges This PR/Issue is related to bridges. labels May 20, 2024

serban300 self-assigned this May 20, 2024

serban300 mentioned this pull request May 20, 2024

Beefy: add runtime support for reporting fork voting #4523

Open

3 tasks

serban300 marked this pull request as ready for review May 20, 2024 13:17

serban300 requested a review from acatangiu as a code owner May 20, 2024 13:17

serban300 requested a review from svyatonik May 20, 2024 13:17

svyatonik approved these changes May 20, 2024

View reviewed changes

serban300 mentioned this pull request May 20, 2024

BEEFY: add support for slashing validators signing forking commitments #1903

Closed

acatangiu approved these changes May 20, 2024

View reviewed changes

substrate/frame/merkle-mountain-range/src/tests.rs Show resolved Hide resolved

acatangiu removed the R0-silent Changes should not be mentioned in any release notes label May 20, 2024

Address code review comments

4b69c20

serban300 mentioned this pull request May 23, 2024

Define OpaqueValue #4550

Merged

serban300 added 2 commits May 24, 2024 14:39

Validate ancestry proofs generated at older blocks

f709cef

Merge remote-tracking branch 'upstream/master' into beefy-equivocatio…

5f4ab3d

…n-runtime

serban300 added 5 commits June 7, 2024 12:34

Add future block voting offence

4f85eaa

Merge remote-tracking branch 'upstream/master' into beefy-equivocatio…

d608795

…n-runtime

Merge branch 'master' into beefy-equivocation-runtime

ede4a01

prdoc

2450e77

fix prdoc

7445ec4

acatangiu approved these changes Jul 3, 2024

View reviewed changes

substrate/frame/beefy/src/equivocation.rs Outdated Show resolved Hide resolved

prdoc/pr_4522.prdoc Outdated Show resolved Hide resolved

Update prdoc

89a78d5

Co-authored-by: Adrian Catangiu <adrian@parity.io>

acatangiu enabled auto-merge July 3, 2024 11:28

check_equivocation_proof -> check_double_voting_proof

80b4c6f

bkontur reviewed Jul 3, 2024

View reviewed changes

prdoc/pr_4522.prdoc Show resolved Hide resolved

bkontur reviewed Jul 3, 2024

View reviewed changes

bkontur approved these changes Jul 3, 2024

View reviewed changes

acatangiu added this pull request to the merge queue Jul 3, 2024

serban300 removed this pull request from the merge queue due to a manual request Jul 3, 2024

franciscoaguirre reviewed Jul 3, 2024

View reviewed changes

franciscoaguirre approved these changes Jul 3, 2024

View reviewed changes

Update prdoc

95c82ff

serban300 enabled auto-merge July 3, 2024 12:36

serban300 disabled auto-merge July 3, 2024 12:53

serban300 added 2 commits July 3, 2024 15:55

Fix semver

e8822ca

Leftover semver fix

fb12a8a

acatangiu reviewed Jul 3, 2024

View reviewed changes

prdoc/pr_4522.prdoc Outdated Show resolved Hide resolved

upate prdoc bumps

c1f97b5

serban300 enabled auto-merge July 3, 2024 13:18

serban300 added this pull request to the merge queue Jul 3, 2024

Merged via the queue into paritytech:master with commit b6f1823 Jul 3, 2024
156 of 159 checks passed

serban300 deleted the beefy-equivocation-runtime branch July 3, 2024 14:12

This was referenced Jul 18, 2024

Relayers to implement bridge BEEFY fork equivocation detection Snowfork/snowbridge#1260

Open

BEEFY: slash validators voting on non-finalized forks #1120

Closed

turuslan mentioned this pull request Aug 2, 2024

beefy report double voting qdrvm/kagome#2169

Merged

This was referenced Aug 21, 2024

Update polkadot-sdk from v1.11.0 to stable2407 moondance-labs/tanssi#659

Open

Update polkadot-sdk from v1.11.0 to stable2407 moonbeam-foundation/moonbeam#2912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEEFY] Add runtime support for reporting fork voting #4522

[BEEFY] Add runtime support for reporting fork voting #4522

serban300 commented May 20, 2024 •

edited

Loading

svyatonik left a comment

svyatonik May 20, 2024

serban300 May 20, 2024

serban300 May 20, 2024 •

edited

Loading

acatangiu May 20, 2024

serban300 May 20, 2024

acatangiu May 21, 2024

serban300 May 21, 2024

serban300 May 24, 2024 •

edited

Loading

Lederstrumpf commented May 21, 2024

serban300 commented May 23, 2024

bkontur Jul 3, 2024

serban300 Jul 3, 2024

franciscoaguirre Jul 3, 2024

serban300 Jul 3, 2024

[BEEFY] Add runtime support for reporting fork voting #4522

[BEEFY] Add runtime support for reporting fork voting #4522

Conversation

serban300 commented May 20, 2024 • edited Loading

svyatonik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Lederstrumpf commented May 21, 2024

serban300 commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 commented May 20, 2024 •

edited

Loading

serban300 May 20, 2024 •

edited

Loading

serban300 May 24, 2024 •

edited

Loading